This repository has been archived by the owner on Oct 7, 2024. It is now read-only.
feat(272): add chainId to Keyring API requests (transaction/typed message)
#231
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ccharly
force-pushed
the
feature/272/chain-id-keyring-api
branch
from
6e77c23 to
d4dd5c7
Compare
danroc
reviewed
Mar 5, 2024
For some reason, the encoding of `type` and `data` are different from what we have. I didn't track down this behavior, so it could be a small "mistake" on our side, or there is different encoding being used somewhere in the flow.
ccharly
force-pushed
the
feature/272/chain-id-keyring-api
branch
from
d4dd5c7 to
b09a4ea
Compare
danroc
approved these changes
Mar 5, 2024
danroc
changed the title
chainId to keyring api requests (transaction/typed message)
danroc
changed the title
chainId to keyring api requests (transaction/typed message)chainId to Keyring API requests (transaction/typed message)
3 tasks
k-g-j
added a commit
to MetaMask/snap-account-abstraction-keyring
that referenced
this pull request
Mar 12, 2024
## Description PR adds more unit tests for specific flows and error messages with the `keyring.ts` file. There is a minor refactor in the createAccount method to check for duplicate accounts. * Fixes [#254](https://app.zenhub.com/workspaces/metamask-accounts-team-v2-64c91cbeaa9d1c00126621fd/issues/gh/metamask/accounts-planning/254) * See: [#244](https://app.zenhub.com/workspaces/metamask-accounts-team-v2-64c91cbeaa9d1c00126621fd/issues/gh/metamask/accounts-planning/244) ## Pre-requisites - [ ] https://github.com/MetaMask/accounts-planning/issues/308 * [x] MetaMask/eth-snap-keyring#231 * [ ] https://github.com/MetaMask/accounts-planning/issues/307 ## Test Coverage Report ### Summary The test coverage has been updated as part of the efforts to increase the code coverage for the `snap-account-abstraction-keyring` module. The tests were executed with `yarn test:coverage --no-cache`, ensuring that cached results did not influence the outcomes. The coverage for the `keyring.ts` file is now above 95%. - **Test Suites**: 1 passed, 1 total - **Tests**: 35 passed, 35 total ### Coverage Overview ```plaintext ---------------------|---------|----------|---------|---------|------------------------- File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s ---------------------|---------|----------|---------|---------|------------------------- All files | 87.58 | 60.63 | 82.45 | 87.75 | snap | 100 | 100 | 100 | 100 | hardhat.config.ts | 100 | 100 | 100 | 100 | snap/src | 92.34 | 67.74 | 87.5 | 92.26 | keyring.ts | 95.67 | 67.92 | 100 | 95.62 | 195,443-445,547-549,562 logger.ts | 75 | 57.14 | 55.55 | 75 | 64,81-92,112-122 permissions.ts | 100 | 100 | 100 | 100 | stateManagement.ts | 62.5 | 100 | 50 | 62.5 | 18-25 snap/src/constants | 93.75 | 0 | 100 | 93.75 | aa-factories.ts | 100 | 100 | 100 | 100 | chain-ids.ts | 100 | 100 | 100 | 100 | chainConfig.ts | 100 | 100 | 100 | 100 | dummy-values.ts | 88.88 | 0 | 100 | 88.88 | 15 entrypoints.ts | 100 | 100 | 100 | 100 | snap/src/utils | 72 | 42.85 | 68.75 | 72.6 | ecdsa.ts | 100 | 100 | 100 | 100 | util.ts | 43.33 | 0 | 50 | 42.85 | 15-27,37-49,105 validation.ts | 89.47 | 80 | 100 | 89.47 | 49,81-84,88 ---------------------|---------|----------|---------|---------|------------------------- --------- Co-authored-by: Gustavo Antunes <[email protected]>
ccharly
added a commit
to MetaMask/utils
that referenced
this pull request
Mar 14, 2024
This adds some new helpers regarding CAIP-2 chain IDs. This is in regard to the on-going work of adding those chain-agnostics IDs into our Snap keyring implementations. Initially those helpers were living on https://github.com/MetaMask/eth-snap-keyring repository, but it feels more natural to have them here. Moreover, we might use them elsewhere. ## Related - MetaMask/eth-snap-keyring#231 --------- Co-authored-by: Elliot Winkler <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is the first step of adding a CAIP-2 scope to internal requests.
It's a "first step" as for now, we cannot get/extract the
chainIdfor some methods. This would require a much bigger refactor (which will come later).signTransactionrequestsThe
chainIdis part of the transaction request, so we extract it from there and attach it to the internal request.signTypedData(when applicable) requestsThe
chainIdis part of thedomainfield (as defined by EIP-712), however, some older versions of theeth_signTypedData_vXmight not have it! In this case, we do not provide thescope.As of today, only those methods do not have the
domain:eth_signTypedDataeth_signTypedData_v1Related issues
Testing
Manual testing
Prerequisites
1. Transaction
eip155:${chainId}(depending on which chain you are using)2. Typed data
eip155:${chainId}(depending on which chain you are using)Test coverage report
Coverage remains at 100%
Jest
Added a new test for CAIP helpers:
yarn jest src/caip.test.tsUpdated test regarding transaction flow:
yarn jest src/SnapKeyring.test.ts